fix(filesystem): write in place to preserve file identity on edits#4515
Open
ayushbunny11 wants to merge 1 commit into
Open
fix(filesystem): write in place to preserve file identity on edits#4515ayushbunny11 wants to merge 1 commit into
ayushbunny11 wants to merge 1 commit into
Conversation
write_file/edit_file used a temp-file+rename strategy for existing files, which replaces the inode on every write and destroys the file's creation time, hard links, and inode-based file watches. On POSIX, write in place instead: open with O_RDWR | O_NOFOLLOW to keep the same symlink protection the rename gave us, fstat to confirm the target is still a regular file, then truncate and write. This trades away crash-atomicity (a crash mid-write can now leave a truncated file, where rename left the original untouched). On win32, keep the rename strategy: O_NOFOLLOW isn't reliably enforced there, and symlink creation already requires elevated privileges by default, so the threat model differs. Fixes modelcontextprotocol#4512
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
write_file/edit_file used a temp-file+rename strategy for existing files, which replaces the inode on every write and destroys the file's creation time, hard links, and inode-based file watches.
On POSIX, write in place instead: open with O_RDWR | O_NOFOLLOW to keep the same symlink protection the rename gave us, fstat to confirm the target is still a regular file, then truncate and write. This trades away crash-atomicity (a crash mid-write can now leave a truncated file, where rename left the original untouched).
On win32, keep the rename strategy: O_NOFOLLOW isn't reliably enforced there, and symlink creation already requires elevated privileges by default, so the threat model differs.
Fixes #4512
Description
write_file/edit_fileused a temp-file+rename strategy for existing files, which replaces the inode on every write and destroys the file's creation time, hard links, and inode-based file watches.On POSIX, write in place instead: open with
O_RDWR | O_NOFOLLOWto keep the same symlink protection the rename gave us,fstatto confirm the target is still a regular file, then truncate and write. This trades away crash-atomicity (a crash mid-write can now leave a truncated file, where rename left the original untouched).On win32, keep the rename strategy:
O_NOFOLLOWisn't reliably enforced there, and symlink creation already requires elevated privileges by default, so the threat model differs.Fixes #4512
Server Details
filesystemwrite_file,edit_file)Motivation and Context
The server's own
get_file_infotool reports a file'sbirthtime, but every edit throughwrite_file/edit_filesilently reset it, because the rename-based write replaced the file's inode. This also broke hard links (a link to the file kept pointing at stale pre-edit content) and confused inode-based file watchers, which saw "delete + create" instead of "modify" on every edit.How Has This Been Tested?
npm run buildpasses (typecheck).npm testpasses — 157/157, including new coverage for the EEXIST in-place path, the non-regular-file rejection, and the win32 rename fallback.birthtimeare preserved across an in-place write, and that opening a symlink withO_NOFOLLOWcorrectly fails withELOOP.edit_fileandwrite_fileagainst a real file with a hard link, and confirmed inode/birthtime stayed constant across edits while the hard link tracked the change.Breaking Changes
None. Tool inputs/outputs are unchanged; this only changes the write mechanism on disk. The only observable behavior difference is that crashes mid-write can now leave a truncated file on POSIX, instead of leaving the original untouched.
Types of changes
Checklist